[HUDI-3900] [UBER] Support log compaction action for MOR tables #5958

suryaprasanna · 2022-06-24T09:23:15Z

What is the purpose of the pull request

This pull request adds support for Log compaction action for MOR table types.

Brief change log

Support log compaction action for MOR tables

Verify this pull request

This change added tests and can be verified as follows:

Added unit tests at hudi-common module to verify the correcting ordering of log blocks.
Added unit tests at file system API level to make sure the log compaction is supported.
Added unit tests at client level to verify the log compaction rewrites are working.
Added stress test to rigorously test the log compaction along with compaction on 2 different tables.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

prasannarajaperumal

Looks good to me overall. Minor comments.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

hudi-common/src/main/java/org/apache/hudi/common/table/log/block/HoodieCommandBlock.java

prasannarajaperumal

Changes to AbstractHoodieLogRecordReader needs a more careful review

nsivabalan

I am done reviewing the source code changes except AbstractHoodieLogRecordReader. will review AHLogRecordReader in a day or two.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieWriteClient.java

...t/hudi-client-common/src/main/java/org/apache/hudi/client/utils/MetadataConversionUtils.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

hudi-common/src/main/java/org/apache/hudi/common/util/CompactionUtils.java

hudi-common/src/main/java/org/apache/hudi/common/table/view/AbstractTableFileSystemView.java

hudi-common/src/main/java/org/apache/hudi/common/table/view/HoodieTableFileSystemView.java

nsivabalan · 2022-08-27T00:21:11Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimelineDiffHelper.java

        return TimelineDiffResult.UNSAFE_SYNC_RESULT;
      }
+
+      // Rather than checking for compaction instants with completed check and a commit file,


trying to see if this is a mandatory fix. can we leave it as is. trying to not make any changes to core flow.

nsivabalan

still half way through reviewing AHLogRecordReader. but prasanna raised a question somewhere, if we can filter out log blocks whose size is greater than some threshold while doing log compaction. wanted to comment on that w/o miss.

As per existing patch, guess its not doable. It will result in wrong data being served. Bcoz, the compacted log block will take higher precedence when compared to any blocks prior to that.

for eg,
LB_1, LB_2, LB_3 and LB_4(LB_1, LB_3)

LB_1, 2 and 3 are regular data log blocks, where as LB_4 is the compacted one. but it includes only LB_1 and LB_3. LB_2 is ignored due to its larger size let's say.

So, while reading records from this file slice and finding the final snapshot of all records,
we will essentialy read LB_4 and then LB_2.
But if incase we have records that was updated in both LB_1 and LB_2. ideally we want the version in LB_2 as the final one. but if we read just LB_4 and LB_2, LB_4 will win which implicitly means LB_1's version will take precedence.

So, as of now, the design assumes that any log compaction will include every log block preceding it. just wanted to remind you folks as well. Can we take another pass through entire patch to ensure this assumption holds good everywhere. if not, it could result in wrong data being served.

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

nsivabalan · 2022-08-27T16:44:26Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

+          String compactedFinalInstantTime = blockTimeToCompactionBlockTimeMap.get(instantTime);
+          if (compactedFinalInstantTime == null) {
+            // If it is not compacted then add the blocks related to the instant time at the end of the queue and continue.
+            instantToBlocksMap.get(instantTime).forEach(block -> currentInstantLogBlocks.addLast(block));


we can use instantsBlocks

instantsBlocks.forEach(block -> currentInstantLogBlocks.addLast(block));

regarding L560.
when we have multiple blocks for a given instantTime, shouldn't we be adding those blocks in reverse order.
For eg,
Lets say, for C1 we have LB_1, LB_2, LB_3. and for C2, we have LB_4.

So, instantToBlocksMap will have 2 entries.
when we process C1, value is going to be LB_1, LB-2, LB_3 right? So, we can't do foreach here. we have to add LB_3, followed by LB_2, followed by LB_1.

is this addressed?

Addressed this comment now.

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

nsivabalan

Done w/ my review on AbstractLogRecordReader as well.
I feel the New logic is tad simpler compared to older one and is manageable/comprehensible. Older one had few corner cases here and there. good job on the entire patch @suryaprasanna 👍

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

nsivabalan · 2022-08-27T17:45:35Z

@prasannarajaperumal : As of now, Surya has completely introduced a new scan() within AbstractLogRecordReader. It was not easy to re-use code from older one and it was getting complicated and hence. If you can take a look and provide any suggestions, would be nice on how to abstract these out. or can we go ahead w/ this duplicated(somewhat) code.

prasannarajaperumal

+1 Thanks @suryaprasanna . Can you please get to the comments from @nsivabalan ? We can land this in soon I hope.

suryaprasanna · 2022-09-07T18:10:49Z

@prasannarajaperumal
My apologies, I was off until today due to some personal reasons. Will work on this PR now and get it merged.

nsivabalan · 2022-09-08T14:00:39Z

hey @suryaprasanna : we also wanted to see if its feasible to not introduce a new action, but re-use the ones for compaction? can you check the feasibility on that?

prasannarajaperumal

@suryaprasanna - Thanks for addressing the comments. Can you look at this specific one?
https://github.com/apache/hudi/pull/5958/files#r956601221
Couple of other minor ones looks like are not handled as well.

@nsivabalan - you mentioned that the new Scan api fixes an other unrelated issue as well. That means we roll out USE_LOG_RECORD_READER_SCAN_V2 to true with priority - correct? Dont have to tie this to log compaction rollout. Tracking this here - https://issues.apache.org/jira/browse/HUDI-4940.
Please update the jira link to the actual issue fixed (or please create one). Thanks.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

.../test/java/org/apache/hudi/client/functional/DataValidationCheckForLogCompactionActions.java

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

Summary: Make useScanV2 value to read log blocks configurable Reviewers: balajee, O955 Project Hoodie Project Reviewer: Add blocking reviewers, PHID-PROJ-pxfpotkfgkanblb3detq! Reviewed By: balajee, O955 Project Hoodie Project Reviewer: Add blocking reviewers JIRA Issues: HUDI-2398 Differential Revision: https://code.uberinternal.com/D8093885

suryaprasanna · 2022-09-30T10:44:41Z

@prasannarajaperumal and @nsivabalan thanks a lot for reviewing the diff. I have addressed all the comments.

nsivabalan

addressed most comments by myself. have 3 to 4 pending to be discussed.

hudi-common/src/main/java/org/apache/hudi/common/config/HoodieMetadataConfig.java

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieAppendHandle.java

nsivabalan · 2022-10-01T18:42:44Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/HoodieAppendHandle.java

+        final String key = keyIterator.next();
+        HoodieRecord<T> record = (HoodieRecord<T>) recordMap.get(key);
+        init(record);
+        // For logCompaction operations all the records are read and written as a huge block.


I have left a comment elsewhere. I am bit confused here. lets discuss this f2f.

nsivabalan · 2022-10-01T18:45:01Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

-      if (isFileGroupInPendingCompaction(fileGroup)) {
+          fileGroup.getAllFileSlices().filter(fs -> !isFileSliceNeededForPendingCompaction(fs)
+              && !isFileSliceNeededForPendingLogCompaction(fs)).iterator();
+      if (isFileGroupInPendingCompaction(fileGroup) || isFileGroupInPendingLogCompaction(fileGroup)) {


sorry, I don't see this being addressed?

if (isFileGroupInPendingCompaction(fileGroup)) { // We have already saved the last version of file-groups for pending compaction Id keepVersions--; }

Infact, you removed isFileGroupInPendingLogCompaction from the if condition and missed to fix the other impl.

...t-common/src/main/java/org/apache/hudi/table/action/compact/CompactionExecutionStrategy.java

hudi-common/src/main/java/org/apache/hudi/common/table/log/HoodieMergedLogRecordScanner.java

.../org/apache/hudi/table/action/compact/plan/generators/BaseHoodieCompactionPlanGenerator.java

nsivabalan · 2022-10-01T20:18:31Z

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java

+          String compactedFinalInstantTime = blockTimeToCompactionBlockTimeMap.get(instantTime);
+          if (compactedFinalInstantTime == null) {
+            // If it is not compacted then add the blocks related to the instant time at the end of the queue and continue.
+            instantToBlocksMap.get(instantTime).forEach(block -> currentInstantLogBlocks.addLast(block));


is this addressed?

hudi-bot · 2022-10-08T01:08:08Z

CI report:

0869c63 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

nsivabalan

LGTM. good job on the patch buddy! Great job.

suryaprasanna force-pushed the log-compaction-dev branch from 089748b to 290d6fd Compare June 24, 2022 09:38

vinothchandar self-assigned this Jun 28, 2022

vinothchandar added the rfc Request for comments label Jun 28, 2022

prasannarajaperumal self-requested a review July 15, 2022 05:55

prasannarajaperumal approved these changes Jul 18, 2022

View reviewed changes

suryaprasanna force-pushed the log-compaction-dev branch from 290d6fd to 7202eb3 Compare July 18, 2022 23:16

xushiyan added big-needle-movers and removed rfc Request for comments labels Jul 30, 2022

xushiyan assigned prasannarajaperumal and xushiyan Aug 15, 2022

xushiyan added the priority:critical Production degraded; pipelines stalled label Aug 15, 2022

prasannarajaperumal approved these changes Aug 16, 2022

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java Outdated Show resolved Hide resolved

prasannarajaperumal reviewed Aug 16, 2022

View reviewed changes

prasannarajaperumal self-requested a review August 16, 2022 09:53

nsivabalan requested changes Aug 27, 2022

View reviewed changes

nsivabalan reviewed Aug 27, 2022

View reviewed changes

nsivabalan requested changes Aug 27, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java Show resolved Hide resolved

prasannarajaperumal approved these changes Sep 5, 2022

View reviewed changes

nsivabalan mentioned this pull request Sep 22, 2022

[HUDI-3919] [UBER] Support out of order rollback blocks in AbstractHoodieLogRecordReader #5341

Closed

5 tasks

suryaprasanna force-pushed the log-compaction-dev branch from 8305b96 to 75cceac Compare September 27, 2022 00:01

prasannarajaperumal reviewed Sep 28, 2022

View reviewed changes

suryaprasanna added 3 commits September 30, 2022 03:18

[HUDI-3900] [UBER] Support log compaction action for MOR tables

b337c3e

Address review comments

4590a5c

suryaprasanna force-pushed the log-compaction-dev branch from 75cceac to 4590a5c Compare September 30, 2022 10:39

addressing minor code feedback

c7a7b3a

nsivabalan reviewed Oct 1, 2022

View reviewed changes

suryaprasanna added 3 commits October 4, 2022 14:15

Address review comments

bfb3668

Fix test case failures

00eefd7

Fix unit tests on file system view

0869c63

nsivabalan approved these changes Oct 9, 2022

View reviewed changes

nsivabalan merged commit 86a1efb into apache:master Oct 9, 2022

[HUDI-3900] [UBER] Support log compaction action for MOR tables #5958

[HUDI-3900] [UBER] Support log compaction action for MOR tables #5958

Uh oh!

Conversation

suryaprasanna commented Jun 24, 2022

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

prasannarajaperumal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prasannarajaperumal left a comment

Choose a reason for hiding this comment

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nsivabalan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nsivabalan commented Aug 27, 2022

Uh oh!

prasannarajaperumal left a comment

Choose a reason for hiding this comment

Uh oh!

suryaprasanna commented Sep 7, 2022

Uh oh!

nsivabalan commented Sep 8, 2022

Uh oh!

prasannarajaperumal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

suryaprasanna commented Sep 30, 2022

Uh oh!

nsivabalan left a comment

nsivabalan left a comment •

edited

Loading